Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix absolute paths #203

Merged
merged 1 commit into from
Apr 23, 2015
Merged

Fix absolute paths #203

merged 1 commit into from
Apr 23, 2015

Conversation

smcgivern
Copy link

This should fix #191 and maybe #196 too.

The relativeTo option was removed at https://github.com/gruntjs/grunt-contrib-cssmin/pull/170/files#diff-f0ee9eb300066e1e51fcfbad9d7a78e1L45 and seemingly never added back.

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2015

/CC @sindresorhus

@@ -1,4 +1,7 @@
'use strict';
function absolutePath(path) {
return __dirname + '/' + path;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use path.join().

@smcgivern
Copy link
Author

Good points. Do you mind if I just amend the existing commit, or would you prefer a separate one?

@XhmikosR
Copy link
Member

XhmikosR commented Apr 6, 2015

@smcgivern: just amend.

@smcgivern smcgivern force-pushed the fix_absolute_paths branch from 81586b2 to 4deac5b Compare April 6, 2015 18:16
@smcgivern
Copy link
Author

OK, done 👍

@jd-carroll
Copy link

I know this is already merged, but..

Why are you setting:

options.relativeTo = path.dirname(availableFiles[0]);

When it should be relative to the destination:

options.relativeTo = options.target = file.dest;

@smcgivern
Copy link
Author

Honestly, because it was like that before:

options.relativeTo = path.dirname(file);
In the case here, where we're using absolute paths, it doesn't look like it matters. A test case that shows why this is the wrong behaviour is probably the best thing to provide.

This also isn't merged - @XhmikosR a little help please? 😃

XhmikosR added a commit that referenced this pull request Apr 23, 2015
@XhmikosR XhmikosR merged commit fe59f98 into gruntjs:master Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grunt-contrib-cssmin creating min.css empty files
5 participants